chore(inputs.elasticsearch_query): Migrate to official ES client#19288
Conversation
|
@alespour regarding the question if we need the v5 client if code works with v6, I'd say no. If you can cover multiple ES server versions with one client, go for it as long as this doesn't limit the queries or anything... |
|
@srebhan Thank you very much for very thorough review. I'm overhauling the PR. |
|
@srebhan PR is overhauled. Plugin uses concrete client implementation for each major version (v5, v6). Version-independent logic is now in shared functions (code blocks moved One thing that requires maintainers decision: v5 client does not support discovery. Now, when I can think of 3 solutions:
|
|
Thanks @alespour for the update, I will review in a minute... Regarding
Please issue a warning and silently ignore discovery for v5. I think this is best as it is unlikely someone is still on 5.x and uses discovery. Maybe also add a short comment in the |
| if interval <= 0 { | ||
| return | ||
| } |
There was a problem hiding this comment.
Can we please check this outside of this function?!
| ctx, cancel := context.WithCancel(context.Background()) | ||
|
|
||
| var wg sync.WaitGroup | ||
| wg.Add(1) | ||
| go func() { |
There was a problem hiding this comment.
I would prefer to do the goroutine setup on the caller side and pass the context in here. This saves the returned function and makes it clear that the client runs another thread...
|
|
||
| // startDiscovery runs node discovery immediately and repeats it when the interval is positive. | ||
| // The returned function stops the loop and waits for any active call to return. | ||
| func startDiscovery(log telegraf.Logger, interval time.Duration, discover func(context.Context) error) func() { |
There was a problem hiding this comment.
Can you please make log the last argument?! We usually do this in other places...
| if cfg.enableSniffer { | ||
| cfg.httpClient.CloseIdleConnections() | ||
| return nil, errors.New("enable_sniffer is not supported by the official ElasticSearch v5 client") | ||
| } |
There was a problem hiding this comment.
Can we please check this on the caller side?
| } | ||
| } | ||
|
|
||
| func (e *ElasticsearchQuery) Start(telegraf.Accumulator) error { |
There was a problem hiding this comment.
Can we please merge this with newClient?!
| mapMetricFields map[string]string | ||
| measurements map[string]map[string]string | ||
| queries interface{} // client specific data to execute the query | ||
| queries interface{} // prepared once and reused across collections |
There was a problem hiding this comment.
[nit] queries is interface{} but it's only ever []queryData, which forces the unchecked assertion at client.go:246 (and the comment there rationalizing the panic). Same shape at client.go:399 where query()'s interface{} return gets asserted back to map[string]json.RawMessage. Both are internal round-trips through the same package, so typing the field as []queryData and the client.query return as map[string]json.RawMessage drops two assertions and the "programming error" caveat. Happy to be argued out of it if the interface{} is deliberate for a later ES7 shape.
There was a problem hiding this comment.
No, interface{} is not needed V7+, with near certainty.
|
Download PR build artifacts for linux_amd64.tar.gz, darwin_arm64.tar.gz, and windows_amd64.zip. 📦 Click here to get additional PR build artifactsArtifact URLs |
Summary
olivere/elastic.Node discovery caveats
As results of official clients limitations:
enable_sniffer = truewill log warning because the official v5 client does not support node discovery, and continue.DiscoverNodes()has no context support. Shutdown waits for it to finish, bounded by the configured HTTP timeout (5s by default).health_check_intervalcontrols how often nodes are discovered whenenable_sniffer = true; olivere-style health checks are no longer performed.Checklist
Related issues
resolves #19212